Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added .Net6 target framework #674

Merged
merged 9 commits into from
Jan 12, 2022
Merged

Added .Net6 target framework #674

merged 9 commits into from
Jan 12, 2022

Conversation

Havunen
Copy link
Contributor

@Havunen Havunen commented Dec 8, 2021

Fixes #675

Adds .net6.0 LTS compilation target and updates the package references to latest version

@dtchepak Could you review this PR please :)

@Havunen
Copy link
Contributor Author

Havunen commented Dec 10, 2021

I have published custom version of this package to nuget as NSubstituteNet6, it can be deprecated when the official package gets .NET 6 support https://www.nuget.org/packages/NSubstituteNet6/ it also fixes the security vulnerabilities mentioned here: #673

Copy link
Member

@dtchepak dtchepak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this PR. LGTM 👍
Will see if @zvirja or @alexandrnikitin have any comments then will merge.

@zvirja
Copy link
Contributor

zvirja commented Dec 14, 2021

@Havunen Would it be possible to keep netstandard 1.3 for now to not cause breaking changes? As otherwise dependent libraries like "AutoFixture" would be broken 😢 We would most likely have to release a new major, but then it's a bit more changes and will take longer.

It would also help if you could drop a bit more info on reasoning of this PR (except you being generous and help to develop the project 😊). Is it blocking you in certain scenarios? This would also help to understand how soon it should be released.

Thank you!

@Havunen
Copy link
Contributor Author

Havunen commented Dec 15, 2021

Would it be possible to keep netstandard 1.3 for now to not cause breaking changes? As otherwise dependent libraries like "AutoFixture" would be broken 😢 We would most likely have to release a new major, but then it's a bit more changes and will take longer.

Ok, I can take a look.

It would also help if you could drop a bit more info on reasoning of this PR (except you being generous and help to develop the project 😊). Is it blocking you in certain scenarios? This would also help to understand how soon it should be released.

I was just trying to remove known security vulnerabilities from our project and then ended up here 😂

@Havunen
Copy link
Contributor Author

Havunen commented Dec 15, 2021

added back netstandard 1.3 target framework

Copy link
Contributor

@zvirja zvirja left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I think we can go ahead and publish a new minor!

@alexandrnikitin
Copy link
Member

@Havunen Thank you for your contribution!

I personally wouldn't update System.* packages unless they affect NSubstitute functionality, even if there are security fixes. NSubstitute is a library, I don't think it should drive the update process (vs a framework like ASP.NET or maybe xUnit). Anyway those libs can be updated directly, the NSub's version upper boundary is open. I can imagine cases when users just can't update those libs.

Revert package version update
@Havunen
Copy link
Contributor Author

Havunen commented Dec 26, 2021

@alexandrnikitin ok I reverted that version upgrade, however it sucks big time to start installing dependencies of dependency at application level. Then one day the dependency refactors given dependency away and the application is still stuck with it, because nobody knows why it exists or why it was added and following these dependencies in each version upgrade is non trivial task in larger projects

@Havunen
Copy link
Contributor Author

Havunen commented Dec 26, 2021

Could this PR be merged and new version published to nuget?

Copy link
Member

@alexandrnikitin alexandrnikitin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thank you!

One minor change for our tests project targets.

@alexandrnikitin
Copy link
Member

@Havunen I understand your concerns regarding dependency management. It's a hard problem to crack and .NET world is not the worst compared to other ecosystems. I believe it should be on package manager's, IDE's or code analyzer's shoulders to solve it. AFAIK latest Visual Studio or R# can help with unused packages.

@dtchepak dtchepak merged commit f890efa into nsubstitute:master Jan 12, 2022
@dtchepak
Copy link
Member

Merged. Thank you for the contribution and review discussion! 🙇

@aboryczko
Copy link

Any info when will this be published to nuget?

dtchepak added a commit to dtchepak/NSubstitute that referenced this pull request Jan 17, 2022
@dtchepak dtchepak mentioned this pull request Jan 17, 2022
dtchepak added a commit that referenced this pull request Jan 24, 2022
Releasing changes from #636 and #674.
@dtchepak
Copy link
Member

@aboryczko This has been published as 4.3.0. Sorry for the delay.

@aboryczko
Copy link

@dtchepak thanks :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add .Net6 target framework
5 participants